Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds a ctx.cache stub property to ExecutionContext, delegating to a virtual Worker::Api::getCtxCacheProperty() hook that returns undefined by default.
Issues (highest severity first):
- [HIGH] New
ctx.cacheproperty is unconditionally exposed with no compat flag or autogate. Adding a new property to everyExecutionContextinstance is a behavioral change — it could shadow user-definedcacheproperties or break code that checks'cache' in ctx. Otherctxproperties (exports,version,abort) are all gated behind compat flags. This needs at minimum a compat flag or autogate for staged rollout. - [MEDIUM] No tests. There are no tests covering the new
getCache()path or thegetCtxCacheProperty()hook. - [LOW] Missing TypeScript type definition. The
JSG_TS_OVERRIDEblocks inJSG_RESOURCE_TYPEdon't includecache, so generated TypeScript types won't reflect this property. - [LOW] Comment inaccuracies. Minor comment issues flagged inline.
These comments were generated by an AI code review assistant and may not be perfect. Please use your judgment when evaluating them.
|
Review posted successfully on PR #6465. Here's a summary of the findings:
|
|
The generated output of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6465 +/- ##
==========================================
+ Coverage 70.95% 71.27% +0.31%
==========================================
Files 428 434 +6
Lines 119850 121333 +1483
Branches 18988 19009 +21
==========================================
+ Hits 85037 86475 +1438
- Misses 23529 23564 +35
- Partials 11284 11294 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tewaro
force-pushed
the
atewari/purge-jsrpc
branch
from
8413fb6 to
5535f5e
Compare
|
@jasnell can you review this PR please, you probably know better than I do the intricacies that are worth watching out for here. |
| } | ||
|
|
||
| jsg::JsValue ExecutionContext::getCache(jsg::Lock& js) { | ||
| // Hook for the embedding application (e.g. edgeworker) to provide a ctx.cache object. |
There was a problem hiding this comment.
We don't use the name edgeworker externally as Akamai has a copyright on the name (that they acquired long after we started using it but... is what it is)
| // Hook for the embedding application (e.g. edgeworker) to provide a ctx.cache object. | |
| // Hook for the embedding application to provide a ctx.cache object. |
| } | ||
|
|
||
| // Returns an optional cache object for Cache-enabled workers. | ||
| // The default implementation returns undefined. Overridden via IoChannelFactory. |
There was a problem hiding this comment.
The default impl in global-scope doesn't just return undefined. It checks to see if there's an active IoContext.
| return props.getHandle(js); | ||
| } | ||
|
|
||
| // Returns an optional cache object for Cache-enabled workers. |
There was a problem hiding this comment.
A bit more detail about the expected return value and where the shape of that object is defined would be helpful. It's a bit unusual for a property to return a generic jsg::JsValue when the return value is expected to be an object. Is it a jsg::Object? Is is a JSG_STRUCT that happens to have a function property? Is it a jsrpc target? etc.
4 participants
No description provided.